Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEAT new target class for AWS Bedrock Anthropic Claude models #699

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

kmarsh77
Copy link

@kmarsh77 kmarsh77 commented Feb 7, 2025

Description

Adding a new target class for AWS Bedrock Anthropic Claude models. It will only work for Anthropic Claude models as the request body is specific to those, but it should be easy to modify the class for use with other Bedrock models.

boto3 Python library is used for sending requests. Local AWS credentials are used for authentication, typically stored in ~/.aws.

Tests and Documentation

Description of target class and parameters is provided in the class file, aws_bedrock_claude_target.py. Unit tests are provided in tests/unit/test_aws_bedrock_claude_target.py.

@kmarsh77
Copy link
Author

kmarsh77 commented Feb 7, 2025 via email

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks very good! I left a few comments, and you likely have complaints from our pre-commit hooks. You can run pre-commit run --all-files to fix that locally.

use of aws_bedrock_claude_target.py target class requires the boto3 library
Deleting unnecessary commented out line
Adding support for multi-turn
@kmarsh77
Copy link
Author

I ran pre-commit run --all-files locally and pushed those changes to my fork.

The only error I couldn't fix is the following:
pyrit/prompt_target/aws_bedrock_claude_chat_target.py:150: error: Dict entry 1 has incompatible type "str": "dict[str, str]"; expected "str": "str" [dict-item]

This is referring to case where an image is sent in a message; the expected format for AWS Bedrock Claude is:
{ "type": "image", "source": { "type": "base64", "media_type": "image/jpeg", "data": "content image bytes" } }

Therefore I'm not sure how to fix this error.

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Just a few small things. We're also getting an account so that we can run this in integration tests soon.



@pytest.mark.asyncio
async def test_send_prompt_async(aws_target, mock_prompt_request):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to skip these if boto3 isn't installed. For HuggingFace chat target we do

def is_torch_installed():
    try:
        import torch  # noqa: F401

        return True
    except ModuleNotFoundError:
        return False

@pytest.mark.skipif(not is_torch_installed(), reason="torch is not installed")
...

Maybe you can do the same? Otherwise it'll fail in the cases where boto3 isn't installed.

Similarly, you can't import boto3 at the top of the file, but you need to import inside a try-except block inside your target constructor. See hugging_face_chat_target.py for an example where we do this with torch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the suggested changes and pushed them to my fork, however note that pre-commit is now complaining that "'boto3' imported but unused". Let me know if I can fix it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added the # noqa: F401 for that (see in the snippet above)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this for the imports in both files, also added the necessary if TYPE_CHECKING block at top of target file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants